Skip to content

test #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

test #1

wants to merge 2 commits into from

Conversation

yongruilin
Copy link
Owner

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@yongruilin yongruilin force-pushed the gke-ai-hackathon-api-review-bot branch from a53a08d to 601924c Compare February 13, 2025 21:58
@yongruilin yongruilin force-pushed the gke-ai-hackathon-api-review-bot branch from 601924c to 17104da Compare February 13, 2025 22:04
@yongruilin
Copy link
Owner Author

Gemini API Review Result:

Okay, I've reviewed the pull request description and the README.md content. Here's a summary and some potential feedback:

Overall Impression:

The PR itself ("test") is extremely minimal. It's essentially just a placeholder PR with an empty body (except for the boilerplate). The README.md content is a copy of the official Kubernetes README.

Observations and Potential Feedback:

  • PR Title and Body: The title "test" is not informative. The body is just a template. This should contain some information about the purpose of the PR. Even a simple test PR should have some goal.
  • Missing PR Kind: No /kind label is added. This is important for categorization.
  • Missing Fixes: Fixes # is there, but is incomplete and does not point to any actual issue.
  • Empty Release Note: The release note block is empty. Given the current content of the PR, this is correct.
  • Duplicated README.md: The inclusion of the standard Kubernetes README.md is odd. There's no indication as to why this is being included, or what changes, if any, are planned for it. Duplicating it like this is not a good pattern unless it's specifically related to the work being done.
  • Lack of Purpose: The biggest issue is that the PR has no discernible purpose. What is being tested? Why is this PR being created?

Recommendations:

  1. Specify the PR type: Add the relevant /kind label based on what the PR intends to do (e.g., /kind cleanup, /kind documentation, /kind feature, or, if it's just a test, /kind other).
  2. Provide a meaningful title: Change the title to something descriptive, for example, "Add basic test framework setup" (if that's the intent).
  3. Explain the purpose in the body: The body must explain what the PR does, why it's needed, and how it addresses a specific problem or goal.
  4. Remove or justify the README.md: If the README.md is there by mistake, remove it. If it's there intentionally, explain why it's included and what modifications are proposed for it in the PR body.
  5. Link to an issue (if applicable): If the PR is intended to fix an issue, link it using Fixes #<issue number>. If not, remove the Fixes # line.

Example Revised PR Description (assuming the intent is to create a basic testing framework setup):

PR Number: 1
Title: Add basic test framework setup

Body:
<!--  Thanks for sending a pull request!  Here are some tips for you: ... -->

#### What type of PR is this?

/kind cleanup
/kind other

#### What this PR does / why we need it:

This PR introduces a basic testing framework setup for [Specify the component].  This includes a minimal test file and necessary dependencies to run unit tests.  This is the first step in improving test coverage and ensuring the stability of the [Specify the component].

#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->

#### Special notes for your reviewer:

This is a preliminary setup. Future PRs will add more tests.  Please focus on the overall structure and integration with the existing codebase.

#### Does this PR introduce a user-facing change?

```release-note
NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Documentation:


With this context, the `README.md` would also need a reason to be included and a summary of changes being proposed for it.

**In summary:** The PR, as it stands, needs significant work to be considered a valid contribution.  It requires a clear purpose, a descriptive title, and a proper explanation in the body.

@yongruilin yongruilin force-pushed the gke-ai-hackathon-api-review-bot branch from 17104da to 30482c4 Compare February 13, 2025 22:16
@yongruilin yongruilin closed this Mar 11, 2025
yongruilin pushed a commit that referenced this pull request Oct 3, 2025
@pohly
DRA ResourceSlice tracker: explain test a bit better, fix -run
02a51e3 
Instead of creating a new test case, the permutation is passed down. This
enables adding the event numbers to the log output, which is useful to
understand better which output belongs to which input:

    === RUN   TestListPatchedResourceSlices/update-patch/2_3_0_1
    tracker.go:396: I0929 14:28:40.032318] event #1: ResourceSlice add slice="s1"
    tracker.go:581: I0929 14:28:40.032404] event #1: syncing ResourceSlice resourceslice="s1"
    tracker.go:659: I0929 14:28:40.032446] event #1: ResourceSlice synced resourceslice="s1" change="add"
    tracker.go:396: I0929 14:28:40.032502] event #2: ResourceSlice add slice="s2"
    tracker.go:581: I0929 14:28:40.032536] event #2: syncing ResourceSlice resourceslice="s2"
    tracker.go:659: I0929 14:28:40.032568] event #2: ResourceSlice synced resourceslice="s2" change="add"
    tracker.go:463: I0929 14:28:40.032609] event #0/#0: DeviceTaintRule add patch="rule"
    tracker.go:581: I0929 14:28:40.032639] event #0/#0: syncing ResourceSlice resourceslice="s1"
    tracker.go:703: I0929 14:28:40.032675] event #0/#0: processing DeviceTaintRule resourceslice="s1" deviceTaintRule="rule"
    tracker.go:807: I0929 14:28:40.032712] event #0/#0: applying matching DeviceTaintRule resourceslice="s1" deviceTaintRule="rule" device="driver1.example.com/pool-1/device-1"
    tracker.go:868: I0929 14:28:40.032780] event #0/#0: Assigned new taint ID, no matching taint resourceslice="s1" deviceTaintRule="rule" device="driver1.example.com/pool-1/device-1" taintID=0 taint="example.com/taint=tainted:NoExecute"
    tracker.go:654: I0929 14:28:40.033023] event #0/#0: ResourceSlice synced resourceslice="s1" change="update" diff=<
        	@@ -23,7 +23,32 @@
        	     "BindingConditions": null,
        	     "BindingFailureConditions": null,
        	     "AllowMultipleAllocations": null,
        	-    "Taints": null
        	+    "Taints": [
        	+     {
        	+      "Rule": {
        	+       "metadata": {
        	+        "name": "rule"
        	+       },
        	+       "spec": {
        	+        "deviceSelector": {
        	+         "pool": "pool-1"
        	+        },
        	+        "taint": {
        	+         "key": "example.com/taint",
        	+         "value": "tainted",
        	+         "effect": "NoExecute",
        	+         "timeAdded": "2006-01-02T15:04:05Z"
        	+        }
        	+       },
        	+       "status": {}
        	+      },
        	+      "ID": 1,
        	+      "key": "example.com/taint",
        	+      "value": "tainted",
        	+      "effect": "NoExecute",
        	+      "timeAdded": "2006-01-02T15:04:05Z"
        	+     }
        	+    ]
        	    }
        	   ],
        	   "Taints": null,
         >
    tracker.go:482: I0929 14:28:40.033224] event #0/#1: DeviceTaintRule update patch="rule" diff=<
        	@@ -4,7 +4,7 @@
        	  },
        	  "spec": {
        	   "deviceSelector": {
        	-   "pool": "pool-1"
        	+   "pool": "pool-2"
        	   },
        	   "taint": {
        	    "key": "example.com/taint",
         >
    tracker.go:581: I0929 14:28:40.033285] event #0/#1: syncing ResourceSlice resourceslice="s1"
    tracker.go:703: I0929 14:28:40.033319] event #0/#1: processing DeviceTaintRule resourceslice="s1" deviceTaintRule="rule"
    tracker.go:654: I0929 14:28:40.033478] event #0/#1: ResourceSlice synced resourceslice="s1" change="update" diff=<
        	@@ -23,32 +23,7 @@
        	     "BindingConditions": null,
        	     "BindingFailureConditions": null,
        	     "AllowMultipleAllocations": null,
        	-    "Taints": [
        	-     {
        	-      "Rule": {
        	-       "metadata": {
        	-        "name": "rule"
        	-       },
        	-       "spec": {
        	-        "deviceSelector": {
        	-         "pool": "pool-1"
        	-        },
        	-        "taint": {
        	-         "key": "example.com/taint",
        	-         "value": "tainted",
        	-         "effect": "NoExecute",
        	-         "timeAdded": "2006-01-02T15:04:05Z"
        	-        }
        	-       },
        	-       "status": {}
        	-      },
        	-      "ID": 1,
        	-      "key": "example.com/taint",
        	-      "value": "tainted",
        	-      "effect": "NoExecute",
        	-      "timeAdded": "2006-01-02T15:04:05Z"
        	-     }
        	-    ]
        	+    "Taints": null
        	    }
        	   ],
        	   "Taints": null,
         >
    tracker.go:581: I0929 14:28:40.033601] event #0/#1: syncing ResourceSlice resourceslice="s2"
    tracker.go:703: I0929 14:28:40.033633] event #0/#1: processing DeviceTaintRule resourceslice="s2" deviceTaintRule="rule"
    ...

Disabling event checking only worked when actually running all sub-tests. When
selectively running only one permutation with -run, the boolean variable was
wrong:

    $ go test -run='.*/^update-patch$' ./staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/
    ok  	k8s.io/dynamic-resource-allocation/resourceslice/tracker

    $ go test -run='.*/^update-patch$/3_2_0_1' ./staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/
    --- FAIL: TestListPatchedResourceSlices (0.01s)
        --- FAIL: TestListPatchedResourceSlices/update-patch (0.00s)
            --- FAIL: TestListPatchedResourceSlices/update-patch/3_2_0_1 (0.00s)

                tracker_test.go:762:
                     	Error Trace:	/nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/tracker_test.go:762
                     	            				/nvme/gopath/src/k8s.io/kubernetes/staging/src/k8s.io/dynamic-resource-allocation/resourceslice/tracker/tracker_test.go:856
                    	Error:      	Not equal:
                    	            	expected: []tracker.handlerEvent{tracker.handlerEvent{event:"add", oldObj:(*api.ResourceSlice)(nil), newObj:(*api.ResourceSlice)(0xc000301d40)}, tracker.handlerEvent{event:"add", oldObj:(*api.ResourceSlice)(nil), newObj:(*api.ResourceSlice)(0xc000346000)}}
                    	            	actual  : []tracker.handlerEvent{tracker.handlerEvent{event:"add", oldObj:(*api.ResourceSlice)(nil), newObj:(*api.ResourceSlice)(0xc0001f9ba0)}, tracker.handlerEvent{event:"add", oldObj:(*api.ResourceSlice)(nil), newObj:(*api.ResourceSlice)(0xc000301d40)}, tracker.handlerEvent{event:"update", oldObj:(*api.ResourceSlice)(0xc000301d40), newObj:(*api.ResourceSlice)(0xc0003dba00)}, tracker.handlerEvent{event:"update", oldObj:(*api.ResourceSlice)(0xc0003dba00), newObj:(*api.ResourceSlice)(0xc000301d40)}, tracker.handlerEvent{event:"update", oldObj:(*api.ResourceSlice)(0xc0001f9ba0), newObj:(*api.ResourceSlice)(0xc0003dbba0)}}

Now permutations are detected automatically based on the indices.

While at it, documentation gets moved around a bit to make reading test cases
easier without going to the implementation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@yongruilin